fix(scout): skip attestation on hosts with no TPM device#2900
Conversation
|
related: #2703 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughRegistration now checks TPM presence before enabling attestation. TPM path handling normalizes device-node inputs, probes kernel TPM nodes, and adds tests for candidate selection and presence checks. ChangesTPM attestation gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/scout/src/tpm.rs (1)
195-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpress these tables through
carbide-test-supportrather than hand-rolled loops.Both additions are textbook total-operation tables, yet they reimplement iteration and labeling manually. The house style mandates the shared helpers:
tpm_device_candidates_casesis a naturalvalue_scenarios!/check_valuestable, andtpm_present_probes_explicit_device_pathcollapses into acheck_valuestable over(input, expected_bool)instead of four discreteassert!s. This keeps failures labeled by case and removes bespoke harness code.♻️ Illustrative shape for the presence probe
#[test] fn tpm_present_probes_explicit_device_path() { check_values(tpm_present, &[ ("device:/dev/null", true), ("/dev/null", true), ("device:/dev/forge_scout_nonexistent_tpm", false), ("/dev/forge_scout_nonexistent_tpm", false), ]); }As per coding guidelines: "Prefer table-driven tests using the
carbide-test-supportcrate withscenarios!... andvalue_scenarios!... Usecheck_cases/check_valuesdirectly when a macro would obscure a table."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/scout/src/tpm.rs` around lines 195 - 224, The two TPM tests are hand-rolled table-driven checks and should use the shared carbide-test-support helpers instead of manual loops and asserts. Replace tpm_device_candidates_cases with a value_scenarios! or equivalent check_values-style table over tpm_device_candidates, and collapse tpm_present_probes_explicit_device_path into a check_values table for tpm_present so each case is labeled consistently. Keep the existing case coverage, but move the iteration and assertion logic into the shared helpers to match the project’s test style.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/scout/src/tpm.rs`:
- Around line 195-224: The two TPM tests are hand-rolled table-driven checks and
should use the shared carbide-test-support helpers instead of manual loops and
asserts. Replace tpm_device_candidates_cases with a value_scenarios! or
equivalent check_values-style table over tpm_device_candidates, and collapse
tpm_present_probes_explicit_device_path into a check_values table for
tpm_present so each case is labeled consistently. Keep the existing case
coverage, but move the iteration and assertion logic into the shared helpers to
match the project’s test style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3ea14b92-1ec2-489b-915c-1d29f05ec3ca
📒 Files selected for processing (2)
crates/scout/src/register.rscrates/scout/src/tpm.rs
|
NICo's current design expects TPM on the host to operate. Seeking input from @ajf on feasibility of removing host-TPM dependency ( i think this would trigger more work on api-server side). |
works fine if you disable attestation in config, this is exactly a case I see in env I have where TPM is not working correctly (HW thing) but with disabled attestation I can get machines to enroll into READY state |
|
This looks like a valid use case, i would like to see NICo to support non TPM hosts if config is set. There is already config in API which allow non TPM devices ( I tested similar change in the past, it works fine. Even if scout would ignore TPM it would break if |
|
/ok to test 09b1d85 |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Gate the attestation block on actual TPM presence so a host with no TPM skips AK/EK setup and sends discovery without AttestKeyInfo, which the API accepts when attestation_enabled is false. Signed-off-by: s3rj1k <evasive.gyron@gmail.com>
3 participants
Gate the attestation block on actual TPM presence so a host with no TPM skips AK/EK setup and sends discovery without AttestKeyInfo, which the API accepts when attestation_enabled is false.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes